feat: HOMO/LUMO NB - #351
Open
VsevolodX wants to merge 6 commits into
Open
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 WalkthroughWalkthroughAdds three Materials Designer workflow notebooks for defect formation, interfacial energy, and HOMO–LUMO/frequency calculations. Updates surface-energy lookup logic, adds total-energy retrieval support, refreshes workflow navigation and links, and removes notebook sanity-check output. ChangesMaterials Designer workflow expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant WorkflowNotebook
participant MaterialsAPI
participant JobAPI
participant PropertiesAPI
User->>WorkflowNotebook: configure workflow parameters
WorkflowNotebook->>MaterialsAPI: load and prepare materials
WorkflowNotebook->>JobAPI: create and submit job
JobAPI-->>WorkflowNotebook: completed job
WorkflowNotebook->>PropertiesAPI: retrieve and visualize computed properties
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
other/materials_designer/workflows/defect_formation_energy.ipynb (1)
458-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
apply_scf_kgridinstead of reimplementing the k-grid patch.This block duplicates the shared
apply_scf_kgridhelper that the surface and interfacial notebooks already call. Reusing it keeps behavior consistent (e.g., thefirst_onlyoption and unit-name handling) and avoids drift.♻️ Suggested change
-# K-grid for the defective-cell SCF. -if SCF_KGRID is not None: - new_context = PointsGridDataProvider(dimensions=SCF_KGRID, isEdited=True).get_context_item_data() - for subworkflow in defect_workflow.subworkflows: - unit = subworkflow.get_unit_by_name(name="pw_scf") - if unit: - unit.add_context(new_context) - subworkflow.set_unit(unit) +# K-grid for the defective-cell SCF. +defect_workflow = apply_scf_kgrid(defect_workflow, scf_kgrid=SCF_KGRID)This also lets you drop the
PointsGridDataProviderimport and addapply_scf_kgridto the imports in this cell.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@other/materials_designer/workflows/defect_formation_energy.ipynb` around lines 458 - 465, Replace the inline SCF k-grid patching block with the shared apply_scf_kgrid helper, preserving the defective workflow and SCF_KGRID inputs. Update the cell imports to add apply_scf_kgrid and remove the now-unused PointsGridDataProvider import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@other/materials_designer/workflows/defect_formation_energy.ipynb`:
- Around line 366-369: Update the RuntimeError message in the defect formation
energy workflow to remove the non-interpolated `{element}` placeholder and
reword the guidance generically for the missing elemental materials, while
preserving the existing missing-material details.
In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb`:
- Around line 514-530: Guard the HOMO/LUMO gap calculation in the
CALCULATE_HOMO_LUMO block after fetching homo_data and lumo_data, checking that
both results are non-empty before indexing their first values. When either
result is empty, emit a clear message and skip the subtraction and formatted gap
output; preserve visualization for any returned data.
- Around line 296-312: Validate the standata lookups and required structure
before composing the workflow in the notebook: ensure the selected base entry
exists and contains the expected HOMO/LUMO configuration, and when both
CALCULATE_HOMO_LUMO and CALCULATE_FREQUENCY are enabled, ensure frequency.json
exists with a valid subworkflows[0] entry. Use specific lookup behavior and
raise clear, actionable errors for missing or malformed entries instead of
allowing KeyError/IndexError or silently omitting frequency calculations;
preserve the existing workflow composition for valid data.
In `@README.md`:
- Line 163: Synchronize the Materials Project API-key guidance by updating the
legacy URL references in README.ipynb and the settings configuration near the
API-key definition to the next-gen Materials Project URL used in README.md. Keep
the guidance consistent across all three locations unless a documented
compatibility reason requires retaining the legacy URL.
In `@src/py/mat3ra/notebooks_utils/core/entity/property/api.py`:
- Around line 96-102: Update find_total_energy_for_material so
source="my_account" filters by the resolved owner account used to create or
resolve the material, rather than always using client.my_account.id. Thread that
account through its callers, including organization accounts selected via
ACCOUNT_ID, while preserving curator ownership handling when the documented
my_account source includes curators.
---
Nitpick comments:
In `@other/materials_designer/workflows/defect_formation_energy.ipynb`:
- Around line 458-465: Replace the inline SCF k-grid patching block with the
shared apply_scf_kgrid helper, preserving the defective workflow and SCF_KGRID
inputs. Update the cell imports to add apply_scf_kgrid and remove the now-unused
PointsGridDataProvider import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5626a0b9-5a13-49d9-903f-cfdec2fad19d
📒 Files selected for processing (16)
.github/workflows/check_links.ymlREADME.mdexamples/assets/README.mdother/materials_designer/workflows/Introduction.ipynbother/materials_designer/workflows/defect_formation_energy.ipynbother/materials_designer/workflows/dielectric_tensor.ipynbother/materials_designer/workflows/homo_lumo_frequency.ipynbother/materials_designer/workflows/interfacial_energy.ipynbother/materials_designer/workflows/phonon_dos_dispersion.ipynbother/materials_designer/workflows/surface_energy.ipynbsrc/py/mat3ra/notebooks_utils/core/entity/material/analysis.pysrc/py/mat3ra/notebooks_utils/core/entity/material/api.pysrc/py/mat3ra/notebooks_utils/core/entity/property/api.pysrc/py/mat3ra/notebooks_utils/workflow.pytests/py/unit/core/entity/test_material_analysis.pytests/py/unit/test_workflow_utils.py
Comment on lines
+366
to
+369
| " raise RuntimeError(\n", | ||
| " f\"Missing elemental reference material(s) for {missing}. \"\n", | ||
| " \"Add elemental material(s) from Standata, or add tag 'elemental' with metadata.element = {element}\"\n", | ||
| " )\n", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
{element} won't interpolate in this error message.
Only the first concatenated string is an f-string; the second is a plain literal, so {element} is printed verbatim (and element isn't in scope here anyway). Reword to avoid the misleading placeholder.
🐛 Suggested fix
raise RuntimeError(
f"Missing elemental reference material(s) for {missing}. "
- "Add elemental material(s) from Standata, or add tag 'elemental' with metadata.element = {element}"
+ "Add the elemental material(s) from Standata, or tag existing ones 'elemental' "
+ "with a matching metadata.element."
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| " raise RuntimeError(\n", | |
| " f\"Missing elemental reference material(s) for {missing}. \"\n", | |
| " \"Add elemental material(s) from Standata, or add tag 'elemental' with metadata.element = {element}\"\n", | |
| " )\n", | |
| " raise RuntimeError(\n", | |
| " f\"Missing elemental reference material(s) for {missing}. \"\n", | |
| " \"Add the elemental material(s) from Standata, or tag existing ones 'elemental' \"\n", | |
| " \"with a matching metadata.element.\"\n", | |
| " )\n", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@other/materials_designer/workflows/defect_formation_energy.ipynb` around
lines 366 - 369, Update the RuntimeError message in the defect formation energy
workflow to remove the non-interpolated `{element}` placeholder and reword the
guidance generically for the missing elemental materials, while preserving the
existing missing-material details.
Comment on lines
+296
to
+312
| "from mat3ra.standata.workflows import WorkflowStandata\n", | ||
| "from mat3ra.wode import Workflow, Subworkflow\n", | ||
| "from mat3ra.notebooks_utils.ipython.entity.workflow.visualize import visualize_workflow\n", | ||
| "\n", | ||
| "nwchem_workflows = WorkflowStandata.filter_by_application(app.name)\n", | ||
| "\n", | ||
| "base_search_term = \"total_energy.json\" if CALCULATE_HOMO_LUMO else \"frequency.json\"\n", | ||
| "workflow = Workflow.create(nwchem_workflows.get_by_name_first_match(base_search_term))\n", | ||
| "\n", | ||
| "if CALCULATE_HOMO_LUMO and CALCULATE_FREQUENCY:\n", | ||
| " frequency_config = nwchem_workflows.get_by_name_first_match(\"frequency.json\")\n", | ||
| " workflow.add_subworkflow(Subworkflow(**frequency_config[\"subworkflows\"][0]))\n", | ||
| "\n", | ||
| "workflow.name = MY_WORKFLOW_NAME\n", | ||
| "\n", | ||
| "visualize_workflow(workflow)\n" | ||
| ] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the NWChem workflow standata to confirm structure/property outputs
fd -e json total_energy frequency -x cat -n {}Repository: mat3ra/api-examples
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -u
echo "== files matching homo/freq/workflow names =="
git ls-files | rg -n 'homo_lumo|frequency|total_energy|standata|workflow' || true
echo
echo "== target cell context =="
sed -n '260,330p' other/materials_designer/workflows/homo_lumo_frequency.ipynb 2>/dev/null || true
echo
echo "== find JSON files with total_energy or frequency =="
git ls-files | rg -n '(^|/)(total_energy|frequency)\.json$|standata|workflows' || trueRepository: mat3ra/api-examples
Length of output: 6136
Guard the standata workflow composition assumptions.
WorkflowStandata JSON entries are external to this repo, so this notebook assumes total_energy.json has the HOMO/LUMO properties/values and frequency.json["subworkflows"][0] exists. Add fail-safes or explicit checks and use more specific lookup behavior, otherwise the workflow can fail with a confusing KeyError/IndexError or silently skip the requested frequencies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb` around lines
296 - 312, Validate the standata lookups and required structure before composing
the workflow in the notebook: ensure the selected base entry exists and contains
the expected HOMO/LUMO configuration, and when both CALCULATE_HOMO_LUMO and
CALCULATE_FREQUENCY are enabled, ensure frequency.json exists with a valid
subworkflows[0] entry. Use specific lookup behavior and raise clear, actionable
errors for missing or malformed entries instead of allowing KeyError/IndexError
or silently omitting frequency calculations; preserve the existing workflow
composition for valid data.
Comment on lines
+514
to
+530
| { | ||
| "cell_type": "code", | ||
| "execution_count": null, | ||
| "id": "40", | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "from mat3ra.notebooks_utils.ipython.entity.property.visualize import visualize_properties\n", | ||
| "\n", | ||
| "if CALCULATE_HOMO_LUMO:\n", | ||
| " homo_data = client.properties.get_for_job(job_id, property_name=\"homo_energy\")\n", | ||
| " lumo_data = client.properties.get_for_job(job_id, property_name=\"lumo_energy\")\n", | ||
| " visualize_properties(homo_data, title=\"HOMO Energy\")\n", | ||
| " visualize_properties(lumo_data, title=\"LUMO Energy\")\n", | ||
| " homo_lumo_gap = lumo_data[0][\"value\"] - homo_data[0][\"value\"]\n", | ||
| " print(f\"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV\")\n" | ||
| ] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against empty HOMO/LUMO property results before indexing.
homo_data[0]["value"] / lumo_data[0]["value"] will raise an unguarded IndexError if get_for_job returns an empty list (e.g., the property wasn't produced for this job). Since this runs unconditionally right after fetching the data, a partial failure here crashes the notebook with a low-signal traceback instead of a clear message.
🛡️ Proposed fix
if CALCULATE_HOMO_LUMO:
homo_data = client.properties.get_for_job(job_id, property_name="homo_energy")
lumo_data = client.properties.get_for_job(job_id, property_name="lumo_energy")
visualize_properties(homo_data, title="HOMO Energy")
visualize_properties(lumo_data, title="LUMO Energy")
- homo_lumo_gap = lumo_data[0]["value"] - homo_data[0]["value"]
- print(f"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV")
+ if homo_data and lumo_data:
+ homo_lumo_gap = lumo_data[0]["value"] - homo_data[0]["value"]
+ print(f"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV")
+ else:
+ print("⚠️ HOMO/LUMO data not available for this job")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| "cell_type": "code", | |
| "execution_count": null, | |
| "id": "40", | |
| "metadata": {}, | |
| "outputs": [], | |
| "source": [ | |
| "from mat3ra.notebooks_utils.ipython.entity.property.visualize import visualize_properties\n", | |
| "\n", | |
| "if CALCULATE_HOMO_LUMO:\n", | |
| " homo_data = client.properties.get_for_job(job_id, property_name=\"homo_energy\")\n", | |
| " lumo_data = client.properties.get_for_job(job_id, property_name=\"lumo_energy\")\n", | |
| " visualize_properties(homo_data, title=\"HOMO Energy\")\n", | |
| " visualize_properties(lumo_data, title=\"LUMO Energy\")\n", | |
| " homo_lumo_gap = lumo_data[0][\"value\"] - homo_data[0][\"value\"]\n", | |
| " print(f\"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV\")\n" | |
| ] | |
| { | |
| "cell_type": "code", | |
| "execution_count": null, | |
| "id": "40", | |
| "metadata": {}, | |
| "outputs": [], | |
| "source": [ | |
| "from mat3ra.notebooks_utils.ipython.entity.property.visualize import visualize_properties\n", | |
| "\n", | |
| "if CALCULATE_HOMO_LUMO:\n", | |
| " homo_data = client.properties.get_for_job(job_id, property_name=\"homo_energy\")\n", | |
| " lumo_data = client.properties.get_for_job(job_id, property_name=\"lumo_energy\")\n", | |
| " visualize_properties(homo_data, title=\"HOMO Energy\")\n", | |
| " visualize_properties(lumo_data, title=\"LUMO Energy\")\n", | |
| " if homo_data and lumo_data:\n", | |
| " homo_lumo_gap = lumo_data[0][\"value\"] - homo_data[0][\"value\"]\n", | |
| " print(f\"HOMO-LUMO gap: {homo_lumo_gap:.4f} eV\")\n", | |
| " else:\n", | |
| " print(\"⚠️ HOMO/LUMO data not available for this job\")\n" | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb` around lines
514 - 530, Guard the HOMO/LUMO gap calculation in the CALCULATE_HOMO_LUMO block
after fetching homo_data and lumo_data, checking that both results are non-empty
before indexing their first values. When either result is empty, emit a clear
message and skip the subtraction and formatted gap output; preserve
visualization for any returned data.
| 3. Optionally, for local Jupyter without OIDC, set legacy API token values in [settings.json](src/py/mat3ra/notebooks_utils/core/api/settings.json). See [Get Authentication Params](examples/system/get_authentication_params.ipynb) for details. API tokens can also be generated in [Account Preferences](https://docs.mat3ra.com/accounts/ui/preferences/api/) on the platform. | ||
|
|
||
| NOTE: The Materials Project API key should be set in `settings.json` and obtained from [https://legacy.materialsproject.org/open](https://legacy.materialsproject.org/open). | ||
| NOTE: The Materials Project API key should be set in `settings.json` and obtained from [https://next-gen.materialsproject.org/api](https://next-gen.materialsproject.org/api). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the remaining Materials Project API-key references.
README.ipynb (Lines 83-91) and src/py/mat3ra/notebooks_utils/core/api/settings.py (Lines 9-18) still point to the legacy URL, so repository guidance is now inconsistent. Update those references or explicitly document why they must remain legacy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 163, Synchronize the Materials Project API-key guidance by
updating the legacy URL references in README.ipynb and the settings
configuration near the API-key definition to the next-gen Materials Project URL
used in README.md. Keep the guidance consistent across all three locations
unless a documented compatibility reason requires retaining the legacy URL.
Comment on lines
+96
to
+102
| query = {"exabyteId": exabyte_id, "slug": "total_energy"} | ||
| if source == "curators": | ||
| query["owner.slug"] = "curators" | ||
| elif source == "my_account": | ||
| query["owner._id"] = client.my_account.id | ||
| elif source != "public": | ||
| raise ValueError(f"Invalid source: {source!r}. Expected 'public', 'curators', or 'my_account'.") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm callers pass ACCOUNT_ID (possibly org) while this fn uses my_account.id
rg -nP --type=py -C2 'find_total_energy_for_material\s*\('
rg -nP --glob '*.ipynb' -C2 'find_total_energy_for_material'
# Inspect the my_account attribute source
rg -nP --type=py -C3 'my_account' src/py/mat3ra/notebooks_utilsRepository: mat3ra/api-examples
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -u
echo "Repo files matching api.py / notebooks_utils:"
git ls-files | grep -E '(^|/)api\.py$|notebooks_utils|\.ipynb$' | sed -n '1,200p'
echo
echo "Find target file:"
fd -ap 'api.py' . | sed -n '1,120p'
echo
echo "Search exact function name all files:"
rg -n -C3 'find_total_energy_for_material' . || true
echo
echo "Search total_energy related:"
rg -n -C2 'total_energy|TotalEnergy|Total Energy|TOTAL' . | sed -n '1,240p' || trueRepository: mat3ra/api-examples
Length of output: 38750
🏁 Script executed:
#!/bin/bash
set -u
echo "Target function:"
sed -n '60,130p' src/py/mat3ra/notebooks_utils/core/entity/property/api.py
echo
echo "Auth/client/account-related definitions and selected account setup:"
rg -n -C3 'selected_account|client =|APIClient|ACCOUNT_ID|ORGANIZATION_NAME|my_account|owner_id' src/py/mat3ra/notebooks_utils core src/py examples other -g '*.py' -g '*.ipynb' | sed -n '1,260p'
echo
echo "Bulk material functions:"
rg -n -C3 'def get_bulk_material|def get_bulk_material_by_crystal' src/py/mat3ra/notebooks_utils
sed -n '1,140p' src/py/mat3ra/notebooks_utils/core/entity/material/api.pyRepository: mat3ra/api-examples
Length of output: 28047
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
import json
from pathlib import Path
targets = [
"other/materials_designer/workflows/interfacial_energy.ipynb",
"other/materials_designer/workflows/surface_energy.ipynb",
"other/materials_designer/workflows/defect_formation_energy.ipynb",
"other/materials_designer/workflows/total_energy.ipynb",
]
for path in targets:
p = Path(path)
print(f"\n--- {path} ---")
data = json.loads(p.read_text())
cells = data.get("cells", [])
for i, cell in enumerate(cells):
source = "".join(cell.get("source", []))
if "ACCOUNT_ID" in source or "BULK_TOTAL_ENERGY_SOURCE" in source or "PRISTINE_TOTAL_ENERGY_SOURCE" in source:
print(f"[cell {i}]")
print(source)
PYRepository: mat3ra/api-examples
Length of output: 14219
Route source="my_account" through the resolved account selector.
The notebooks resolve and create materials under ACCOUNT_ID, which can be an organization account when ORGANIZATION_NAME is set, but find_total_energy_for_material(..., source="my_account") queries only client.my_account.id. This makes total-energy lookups fail even though the matching bulk/material was resolved under a different owner, breaking surface, interfacial, and defect formation energy notebooks. Thread the requested owner account into this lookup, including curators when the source documentation says my_account is “curators' or your own”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/py/mat3ra/notebooks_utils/core/entity/property/api.py` around lines 96 -
102, Update find_total_energy_for_material so source="my_account" filters by the
resolved owner account used to create or resolve the material, rather than
always using client.my_account.id. Thread that account through its callers,
including organization accounts selected via ACCOUNT_ID, while preserving
curator ownership handling when the documented my_account source includes
curators.
…energy If MATERIAL_NAME isn't found in the uploads folder, query the platform via client.materials.list(owner._id=ACCOUNT_ID) instead of failing outright. Lets a Cypress test point MATERIAL_NAME at a fixture-uploaded material without needing it bundled into the JupyterLite build. Default is still "H2O" from the uploads folder, unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
other/materials_designer/workflows/homo_lumo_frequency.ipynb (3)
179-181: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate the default-project lookup.
If the selected account has no default project,
projects[0]raisesIndexError. Raise an actionable error before indexing the response.Proposed fix
projects = client.projects.list({"isDefault": True, "owner._id": ACCOUNT_ID}) +if not projects: + raise ValueError(f"No default project exists for account '{ACCOUNT_ID}'.") project_id = projects[0]["_id"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb` around lines 179 - 181, Validate that the result of client.projects.list in the default-project lookup is non-empty before accessing projects[0]. Raise an actionable error identifying that no default project was found for the selected account, while preserving the existing project_id assignment and usage for successful lookups.
365-378: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate cluster selection before creating
Compute.If no clusters are available, Line 371 raises
IndexError. IfCLUSTER_NAMEhas no match, Line 374 passesNonetoCompute. Fail with a clear configuration error in both cases.Proposed fix
clusters = client.clusters.list() print(f"Available clusters: {[c['hostname'] for c in clusters]}") +if not clusters: + raise ValueError("No clusters are available for the selected account.") # Select cluster: use specified name if provided, otherwise use first available if CLUSTER_NAME: cluster = next((c for c in clusters if CLUSTER_NAME in c["hostname"]), None) + if cluster is None: + raise ValueError(f"No cluster matches CLUSTER_NAME='{CLUSTER_NAME}'.") else: cluster = clusters[0]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb` around lines 365 - 378, Validate the cluster selected by the cluster-selection block before constructing Compute: raise a clear configuration error when clusters is empty or when CLUSTER_NAME produces no match. Preserve the existing first-cluster fallback and only pass a confirmed cluster object to Compute.
65-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject an empty calculation selection.
If both toggles are
False, Line 293 selectsfrequency.json. The notebook then submits a frequency workflow even though no calculation was selected. The result cell also skips frequency-specific output.Proposed fix
if CALCULATE_FREQUENCY: calculation_labels.append("Frequency") +if not calculation_labels: + raise ValueError( + "Set CALCULATE_HOMO_LUMO or CALCULATE_FREQUENCY to True." + ) MY_WORKFLOW_NAME = " + ".join(calculation_labels) + " (nwchem)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb` around lines 65 - 74, Update the calculation-selection setup around CALCULATE_HOMO_LUMO and CALCULATE_FREQUENCY to reject the case where both toggles are False before constructing MY_WORKFLOW_NAME or submitting a workflow. Ensure the notebook raises a clear error instead of falling back to frequency.json, while preserving the existing behavior when either calculation is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb`:
- Around line 211-219: Update the material lookup in the workflow’s
platform-loading logic to anchor the escaped name regex at both the beginning
and end, ensuring only an exact case-insensitive name match is returned. Keep
the existing owner filter, no-match error, and matches[0] handling unchanged.
---
Outside diff comments:
In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb`:
- Around line 179-181: Validate that the result of client.projects.list in the
default-project lookup is non-empty before accessing projects[0]. Raise an
actionable error identifying that no default project was found for the selected
account, while preserving the existing project_id assignment and usage for
successful lookups.
- Around line 365-378: Validate the cluster selected by the cluster-selection
block before constructing Compute: raise a clear configuration error when
clusters is empty or when CLUSTER_NAME produces no match. Preserve the existing
first-cluster fallback and only pass a confirmed cluster object to Compute.
- Around line 65-74: Update the calculation-selection setup around
CALCULATE_HOMO_LUMO and CALCULATE_FREQUENCY to reject the case where both
toggles are False before constructing MY_WORKFLOW_NAME or submitting a workflow.
Ensure the notebook raises a clear error instead of falling back to
frequency.json, while preserving the existing behavior when either calculation
is enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c81aef4-aee8-4f68-a791-31d7aea8f415
📒 Files selected for processing (2)
other/materials_designer/uploads/H2O.jsonother/materials_designer/workflows/homo_lumo_frequency.ipynb
Comment on lines
+211
to
+219
| " matches = client.materials.list({\n", | ||
| " \"name\": {\"$regex\": re.escape(name), \"$options\": \"i\"},\n", | ||
| " \"owner._id\": ACCOUNT_ID,\n", | ||
| " })\n", | ||
| " if not matches:\n", | ||
| " raise ValueError(f\"No material containing '{name}' was found in '{FOLDER}' or on the platform.\")\n", | ||
| " material = Material.create(matches[0])\n", | ||
| " print(f\"♻️ Loaded '{name}' from platform: {matches[0]['_id']}\")\n", | ||
| " return material\n", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use an exact and unambiguous platform material lookup.
The unanchored regex can match a different material, such as H2O2 for MATERIAL_NAME = "H2O". matches[0] then submits a job for an arbitrary returned material.
Proposed fix
matches = client.materials.list({
- "name": {"$regex": re.escape(name), "$options": "i"},
+ "name": {"$regex": f"^{re.escape(name)}$", "$options": "i"},
"owner._id": ACCOUNT_ID,
})
if not matches:
raise ValueError(f"No material containing '{name}' was found in '{FOLDER}' or on the platform.")
+ if len(matches) > 1:
+ raise ValueError(f"Multiple platform materials are named '{name}'. Select one explicitly.")
material = Material.create(matches[0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| " matches = client.materials.list({\n", | |
| " \"name\": {\"$regex\": re.escape(name), \"$options\": \"i\"},\n", | |
| " \"owner._id\": ACCOUNT_ID,\n", | |
| " })\n", | |
| " if not matches:\n", | |
| " raise ValueError(f\"No material containing '{name}' was found in '{FOLDER}' or on the platform.\")\n", | |
| " material = Material.create(matches[0])\n", | |
| " print(f\"♻️ Loaded '{name}' from platform: {matches[0]['_id']}\")\n", | |
| " return material\n", | |
| " matches = client.materials.list({\n", | |
| " \"name\": {\"$regex\": f\"^{re.escape(name)}$\", \"$options\": \"i\"},\n", | |
| " \"owner._id\": ACCOUNT_ID,\n", | |
| " })\n", | |
| " if not matches:\n", | |
| " raise ValueError(f\"No material containing '{name}' was found in '{FOLDER}' or on the platform.\")\n", | |
| " if len(matches) > 1:\n", | |
| " raise ValueError(f\"Multiple platform materials are named '{name}'. Select one explicitly.\")\n", | |
| " material = Material.create(matches[0])\n", | |
| " print(f\"♻️ Loaded '{name}' from platform: {matches[0]['_id']}\")\n", | |
| " return material\n", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@other/materials_designer/workflows/homo_lumo_frequency.ipynb` around lines
211 - 219, Update the material lookup in the workflow’s platform-loading logic
to anchor the escaped name regex at both the beginning and end, ensuring only an
exact case-insensitive name match is returned. Keep the existing owner filter,
no-match error, and matches[0] handling unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes